-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: wire up Cypress Studio #23413
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
@@ -148,7 +148,7 @@ describe('Cypress In Cypress E2E', { viewportWidth: 1500, defaultCommandTimeout: | |||
}) | |||
|
|||
it('shows a compilation error with a malformed spec', { viewportHeight: 596, viewportWidth: 1000 }, () => { | |||
const expectedAutHeight = 500 // based on explicitly setting viewport in this test to 596 | |||
const expectedAutHeight = 456 // based on explicitly setting viewport in this test to 596 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the <a>
for the AUT URL to be an <input
> so we can type into it to navigate to a URL for Cypress Studio, and for some reason the wrapping break points for flex-wrap
is slightly different. I have no idea why and was quite stuck on this, but for all intensive purposes the UI is the same, except things wrap in the AUT header slightly earlier if you have your Cypress window really small.
We can probably revisit this in the final pass if needed, but I don't think it makes any difference.
@@ -243,7 +243,7 @@ describe('Cypress in Cypress', { viewportWidth: 1500, defaultCommandTimeout: 100 | |||
cy.visitApp() | |||
cy.contains('dom-content.spec').click() | |||
|
|||
cy.contains('http://localhost:4455/cypress/e2e/dom-content.html').should('be.visible') | |||
cy.findByTestId('aut-url-input').invoke('val').should('contain', 'http://localhost:4455/cypress/e2e/dom-content.html') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit more specific about what's going on, just a general improvement.
@@ -148,7 +148,7 @@ describe('Cypress In Cypress E2E', { viewportWidth: 1500, defaultCommandTimeout: | |||
}) | |||
|
|||
it('shows a compilation error with a malformed spec', { viewportHeight: 596, viewportWidth: 1000 }, () => { | |||
const expectedAutHeight = 500 // based on explicitly setting viewport in this test to 596 | |||
const expectedAutHeight = 456 // based on explicitly setting viewport in this test to 596 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change <a>
to <input>
for the Studio URL input - this caused the breakpoint for flex: flex-wrap
to change slightly. Seems like it's not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start! Didn't do too deep a dive but it looks like your goal of getting most of it wired up is working. Just a few comments, since it's WIP I'll leave it up to you to address them or not
} | ||
}, | ||
|
||
setFileDetails (fileDetails) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to save the file as it fails with error:
ENOENT: no such file or directory, open '/Users/zachjw/work/component-test-repos/create-next-app/create-next-app/cypress/e2e/passing.cy.js'
Notice the projectName is being appended twice create-next-app/create-next-app
. I traced it down to stripCustomProtocol, might need to tweak the regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I am not seeing this. I may need help to reproduce this one.
packages/reporter/src/test/test.tsx
Outdated
<WandIcon /> | ||
</a> | ||
</Tooltip>, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for _launchStudio
below:
I wasn't able to get studio to launch unless I clicked the icon, navigated away from the runner and then back in. I refactored the function to set the studioActive
property and it started working
_launchStudio = (e: MouseEvent) => {
e.preventDefault()
e.stopPropagation()
const { model, events } = this.props
action('studio:init:test', () => {
appState.setStudioActive(true)
events.emit('studio:init:test', model.id)
})()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to reproduce this one. Let's sync up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to debug around a bit like Zach to get studio to activate, but once I figured out the trick I was able to record & save a couple specs without issue. Awesome work getting this much working so quickly!
@mike-plummer @ZachJW34 it sounds like you've both encountered a bug that I am not able to reproduce. I'm going to address all the other comments for now (mainly refactors/tweaks) and we can sync up so I can see how to reproduce the "cannot activate studio" problem you've both encountered. Hmm what URL are you both entering? Edit: pushing a few commits now anyway - may solve the issue, let's see! Thanks for the quick review. |
New demo: new-demop.mp4 |
@lmiller1990 It's working for me after pulling down your latest commits 🥳 . I didn't dig too far into it on Friday but I think the issue was something around the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have very little test coverage for studio and we should add these as cypress-in-cypress tests before this is released. We are missing coverage in the app, launchpad and server (no socket tests for studio). We should also add tests to verify the studio json stats are correctly being read/written with file paths/OS.
@@ -1,5 +1,5 @@ | |||
import { observable } from 'mobx' | |||
import { TestState } from '../test/test-model' | |||
import { Instrument, TestState } from '@packages/types' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@packages/types
needed added in the package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
@@ -63,6 +69,7 @@ const Suite = observer(({ eventManager = events, model }: SuiteProps) => { | |||
export interface RunnableProps { | |||
model: TestModel | SuiteModel | |||
appState: AppState | |||
experimentalStudioActive: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: name-spacing this with experimental
will require updating this when this comes out of experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point, maybe we just name it studioEnabled
?
I'll think about this a bit more.
Hmm actually, maybe we just do a check against Cypress.config('experimentalStudio')
and drop this prop entirely? This seems to be the pattern for experimentalSessionAndOrigin
(using Cypress.config
). *Edit: this is the pattern for driver, reporter never accesses the Cypress
global).
I might just change this to studioEnabled
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to studioEnabled
.
@@ -87,7 +87,9 @@ describe('experimentalSingleTabRunMode', () => { | |||
}) | |||
}) | |||
|
|||
describe('experimentalStudio', () => { | |||
// TODO: Figure out this. experimentalStudio is back, but must be nested under E2E? Or, does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for this? & why are these tests skipped now? I'd expect them to run & pass before this merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the issue number to this TODO: #23338
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is #23338
Marked as blocking Studio release!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's done here #23506
packages/reporter/src/test/test.tsx
Outdated
@@ -106,6 +106,7 @@ interface TestProps { | |||
runnablesStore: RunnablesStore | |||
scroller: Scroller | |||
model: TestModel | |||
experimentalStudioEnabled: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default prop value for experimentalStudioEnabled
should be added as false
throughout the reporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought that as long as we set the top level prop in <Reporter />
it'll get passed down - eg, since it's not optional, we don't need a default value? And TS should complain if we forgot to pass it down somewhere -- or maybe the TS config for reporter
isn't strict enough?
I'll take a look but I don't think setting a default value for a non nullable boolean prop makes sense, does it?
Will definitely get a chance to re-assess once thing final PR opens, but it seems like we should only need default values for nullable props.
model={model} | ||
appState={appState} | ||
experimentalStudioEnabled={false} | ||
/> | ||
</div>) | ||
|
||
cy.percySnapshot() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect tests added for when studio is enabled. When is that work planned if it's not happening here? There are quite a skipped tests in the reporter atm for studio. Here's a few I know off-hand
- https://github.com/cypress-io/cypress/blob/develop/packages/reporter/cypress/e2e/runnables.cy.ts#L174
- https://github.com/cypress-io/cypress/blob/develop/packages/reporter/cypress/e2e/tests.cy.ts#L137
- https://github.com/cypress-io/cypress/blob/develop/packages/reporter/cypress/e2e/suites.cy.ts#L133
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I did not realize we had these skipped - I'll go back and add them in now in #23461
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,118 @@ | |||
<template> | |||
<div v-if="!studioStore.url && studioStore.isActive"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing tests for this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thanks for calling this out -- DM'd you, gonna merge this to the feature branch and add a bunch of tests here: #23461
Tried out your branch. It wasn't working for me: Screen.Recording.2022-08-22.at.9.29.02.AM.movAlso noticing we don't have the experiment listed in the settings page and the config option is not shown in the resolved settings: We are also missing the setting's localized strings as well. The last two experiments we releases had them missing 😅 |
@emilyrohrbough I think we're going to handle adding the flag back with this issue |
{model.type === 'test' ? <Test model={model as TestModel} /> : <Suite model={model as SuiteModel} />} | ||
{model.type === 'test' | ||
? <Test model={model as TestModel} experimentalStudioEnabled={experimentalStudioActive} /> | ||
: <Suite model={model as SuiteModel} experimentalStudioActive={experimentalStudioActive} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use experimentalStudioEnabled
for this prop name. This property describes whether or not Studio is enabled in the Cypress configuration, not whether Studio is currently active. We should be careful not to conflate the two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, globally renamed to studioEnabled
. I also dropped the experimental
prefix, which doesn't add much value internally as pointed out by https://github.com/cypress-io/cypress/pull/23413/files#r951467056
@lmiller1990 UI is now working for me. Also, the problem with saving the file seems to be only related to Next.js apps, it's working on other applications just fine now. What I'm seeing for Next is that the Not a blocker since I know your goal is to try and get close to a 1-to-1 implementation of the previous studio but would be good to followup on this. |
@emilyrohrbough Yep, we have an entire ticket for writing a bunch of tests before moving the feature branch to review: #23461 (Note this branch is targeting said feature branch).
That is weird, I wonder what's going on 🤔 stupid question, I wonder if you need to Oh it's likely around a modal that's supposed to show but isn't - I think I got my appPreferences into some weird state where it says "showedStudioModal: true" but yours is still false (and since the modal isn't implemented, it never shows). This should be addressed by the UI update when it lands. Either way, I will fix your basic comments - for tests and for the |
@ZachJW34 thanks for calling out Next.js weirdness, going to triage this separately -- I think 9.x suffers the same problem. |
I unskipped the skipped tests in In c7d66f5 there was some race condition I think - I just moved the tests around (reorder) and it's working fine now. I am not sure if this was originally flaky or something changed. Either way, these tests are only really unit/integration - I'm excited to add some real E2E tests next using Cypress in Cypress. |
Ok, gonna merge this up and continue working on tests in #23461. |
* chore: wire up Cypress Studio (#23413) * wip * wip * wip - spike * more wip [skip ci] * update style * fix ts * move types around * extract types * lint * fixing tests * fix component test * skip some tests * do not error on experimentalStudio flag * add studio controls placeholder * fixing tests * revert * revert changes * rename store * rename method * remove comment * refactor * correctly feature flag studio * simplify code * simplify code * lift check into useEventManager * correctly hide create studio prompt based on flag; * remove superfulous css * rename variables * fix bugs * wip * unskip tests * unskip more tests * fix a bug in the assertion API * fix bug in assertions [skip ci] * wip - bugs [skip ci] * feat: add experimentalStudio flag back (#23506) Co-authored-by: astone123 <adams@cypress.io> * chore: Add Studio UI to Cypress 10 (#23537) * wip * wip * wip - spike * more wip [skip ci] * update style * fix ts * move types around * extract types * lint * fixing tests * fix component test * skip some tests * do not error on experimentalStudio flag * add studio controls placeholder * fixing tests * revert * revert changes * rename store * rename method * remove comment * refactor * correctly feature flag studio * chore: wip add barebones studio modals * simplify code * simplify code * lift check into useEventManager * correctly hide create studio prompt based on flag; * remove superfulous css * chore: style studio toolbar * chore: misc feedback * chore: remove studio store prop * chore: studio URL prompt and other changes * update component * chore: UI styling and remove studio init modal * chore: revert unnecessary changes * chore: fix types * chore: fix some tests, minor refactor (#23545) * fix test * fix test * add noHelp link to StandardModal Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com> * test: studio e2e tests (#23546) * add basic e2e test * add some e2e tests for studio and a note on limitations * additional spec * add more tests, refactor helper * fix bug in studio * remove test code * chore: UI feedback * fix race condition * update tests * rename test * improve types in reporter * remove dead code * improve tests * merge tests into one spec * chore: Cap instruction modal width; exit studio mode when new spec is chosen * chore: Only render studio error when test has failed; add test for studioEnabled * correctly check if command is studio or not * improve specs and hopefully reduce flake * communicate studio state from app->reporter * receive studio save state validity from app * fix test * improve test coverage * fix external link Co-authored-by: astone123 <adams@cypress.io>
Note targeting a feature branch,
feat/studio-cypress-10
.User facing changelog
N/A - internal for now
Additional details
Wire up Cypress Studio in Cypress 10. There's a bit going on, and it's not fully complete but works at a basic level. It's going into a feature branch, so things like tests, some cleanup etc will come before we merge the whole thing into develop.
Learn about Studio: https://docs.cypress.io/guides/references/cypress-studio#What-you-ll-learn
Steps to test
WIP, there's probably lots of edge cases that are kind of busted so I wouldn't go too far. Here's a demo of how it works, you are welcome to play around and try it out!
The final review will be towards the end once we've got a proper UI in place. It's hard to really review without the full Studio UI in place - that's why it's targeting a feature branch.
Here's a quick demo (not sure why the test fails, but the wiring is working - you can see it generated a spec from my actions).
studi0demo.mp4
Any small bugs or minor fixes will be done in #23461, which is the feature branch. The final review of this is where we need to be more thorough and deliberate.
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?